I'm not sure how to handle

It would require a migration (if the null=True) gets dropped and might break something (don't know for sure tho)

I'm not sure how to handle

Warning

DJ001 Avoid using null=True on string-based fields such as CharField

It would require a migration (if the null=True) gets dropped and might break something (don't know for sure tho)

I think i agree with DJ001 per se as well as with your reasoning why this might need more consideration.

Let's deactivate this one for now and defer it to future us with a new issue.

edit: link for TODO comment: adfinis/timed#23

I'm not sure how to handle
Warning
DJ001 Avoid using null=True on string-based fields such as CharField
It would require a migration (if the null=True) gets dropped and might break something (don't know for sure tho)

I think i agree with DJ001 per se as well as with your reasoning why this might need more consideration.

Let's deactivate this one for now and defer it to future us with a new issue.

edit: link for TODO comment: adfinis/timed#23

That's DB design dogma however. I do agree with it in the general case, but I think that check just goes too far. As you noted, we can do it in a separate PR sometimes, but just for the linter's sake, it's not the right thing to do

timed/subscription/admin.py:27:38: S324 Probable use of insecure hash functions in hashlib: md5
|
25 | password = self.cleaned_data.get("password")
26 | if password is not None:
27 | self.instance.password = hashlib.md5(password.encode()).hexdigest()
| ^^^^^^^^^^^ S324
28 | return super().save(commit=commit)
|

class CustomerPassword(models.Model):
    """Password per customer used for login into SySupport portal.

    Password are only hashed with md5. This model will be obsolete
    once customer center will go live.
    """

Can this be dropped, respectively ignored and dropped in a separate PR?

Since https://github.com/adfinis/customer-center does exist

timed/subscription/admin.py:27:38: S324 Probable use of insecure hash functions in hashlib: md5 | 25 | password = self.cleaned_data.get("password") 26 | if password is not None: 27 | self.instance.password = hashlib.md5(password.encode()).hexdigest() | ^^^^^^^^^^^ S324 28 | return super().save(commit=commit) |

class CustomerPassword(models.Model):
    """Password per customer used for login into SySupport portal.

    Password are only hashed with md5. This model will be obsolete
    once customer center will go live.
    """

Can this be dropped, respectively ignored and dropped in a separate PR?

Since https://github.com/adfinis/customer-center does exist

I'm not 100% if this is still in use or not. It looks like it was part of the old SySupport portal and might have been replaced with a proper solution when the portal was migrated to being an Ember based frontend.

If this is in fact the case then we should create an issue to track the codes removal. Maybe @winged or @trowik know if it's still needed?

timed/subscription/admin.py:27:38: S324 Probable use of insecure hash functions in hashlib: md5 | 25 | password = self.cleaned_data.get("password") 26 | if password is not None: 27 | self.instance.password = hashlib.md5(password.encode()).hexdigest() | ^^^^^^^^^^^ S324 28 | return super().save(commit=commit) |

class CustomerPassword(models.Model):
    """Password per customer used for login into SySupport portal.

    Password are only hashed with md5. This model will be obsolete
    once customer center will go live.
    """

Can this be dropped, respectively ignored and dropped in a separate PR?
Since https://github.com/adfinis/customer-center does exist

I'm not 100% if this is still in use or not. It looks like it was part of the old SySupport portal and might have been replaced with a proper solution when the portal was migrated to being an Ember based frontend.

If this is in fact the case then we should create an issue to track the codes removal. Maybe @winged or @trowik know if it's still needed?

I'm also not 100% sure, but I'd say it's safe to remove it in another PR.

#1049 (comment)

@winged this only works with

from __future__ import annotations

See PEP 563

TIL!

But I think that future thingy is more about forward declarations, not about not having to import stuff that you can import. Do weird things only if you MUST, we shouldn't blindly follow dogma

TIL!

Um, same :)

I promised @c0rydoras that i wold do some light catching up on PEPs wrt to typing, specifically wrt to the previously unknown to me string form, and i discovered a rabbit hole of PEPs that i have yet to go down fully having previously focused on getting started with typing and not deep typing.

I still have some reading to do, while i'm not (yet?) convinced that the string for is pythonic, i do think TYPE_CHECKING has value beyond just being dogma. tbh, i only figured this out when using it.

Basically, the import section has long been a part of my code that i do inspect, but i rarely write it on my own. either my IDE just add the required imports or i write them in any (isort violating) order and ruff fixes whatever i am too lazy to consider at the time of writing. Given this hand-off approach to handling imports, the TYPE_CHECKING block seems to help me have a deeper understanding of the code i write.

I've run into situations where i refactor the code and a dependency was "ruffed" to move my import into the block, this helps demonstrate that i was successful at reducing the coupling between the component under refactor and what i was working on.

Anyways.... @c0rydoras proposed to today that he would love to merge this in the near term and followup on typing discussions in #1054 and I kinda agree with him.

I think this discussion is highly valuable and don't consider it bike-shedding at all. In my opinion we can start safely using some typing without the resulting code being highly un-pythonic. I created #1054 because i see considerable value where typing can help us get rid of several low-level incorrectness's in our code base.

For this PR to get merged ASAP, which i am highly favorable of since it seems to be correct and not introduce and defects i think we have two options:

• Remove any disputable typing related changes and follow up discussing them in Introduce type hinting #1054
• Merge them as is and follow up discussing them in Introduce type hinting #1054

To no surprise i'm in favour of the latter option. The plan is to tackle #1054 right once this is landed.

We should also prepare the typing discussion so we can find consensus/input with a broader audience of stake holders to any policy here.

langer rede kurzer sinn, 🥺 👉 👈

does this address your concerns?

I absolutely agree with you @hairmare - The thing I'm most scared about with this strict Ruff config is that we stop questioning the rules in cases where they don't make sense, and apply patterns just to satisfy the linter.

We've seen this a few times in this PR (not blaming @c0rydoras at all): It's often difficult to see if a linter complaint is a valid criticism or something that should be acceptable in this particular situation.

So in a weird sense having stricter linting solves a problem but introduces a new one: While earlier you had to rely on experience/knowledge to figure out whether a code structure/pattern/whatever is "good", you now have to rely on experience/knowledge/common-sense to figure out whether a linter complaint is "good". Not sure which one is preferrable. For example the whole tuple-vs-list discussion we had here: I think tuples are harder to read and easier to make mistakes with, for the (slight) benefit of them being readonly (but still replaceable)

Regarding type hints - I think it's a glorious idea, and it makes my life (via IDE/LSP/tooling) much better, but: Python will never be a strictly, fully-typed language, so I'm quite happy to make the move rather gradual, and omit it where it's just cluttering up the code without real benefit.

So, in conclusion:

• More, stricter linters are good, but do not replace common sense / experience.
• Type hints are good, but don't be dogmatic about it

I'm OK with merging this PR soon-ish - I'll have a final look over it and approve if I don't see any more issues

@winged how would you merge this?

I'd be in favour of squashing it and then using the fix scope

@winged how would you merge this?

I'd be in favour of squashing it and then using the fix scope

You have my approval, and @hairmare's as well - go ahead 👍